rust/lance-index/src/scalar/inverted/documents.rs (1)

631-657: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid copying the entire address column in the bulk branch.

address_column.values().to_vec() materializes all num_docs addresses into a new Vec even though the bulk branch only indexes the (small) top-k doc_ids. On large partitions this is a needless full-column copy (e.g. ~80 MB for 10M docs) on the final resolution path. values() already yields a &[u64] you can index directly; only the point branch needs an owned copy, and it can zip the slice by reference.

♻️ Proposed change to drop the full-column copy

-        let addresses = address_column.values().to_vec();
+        let addresses = address_column.values();
         if use_bulk && addresses.len() != self.num_docs {
             return Err(corrupt_docs(
                 &self.path,
                 format!(
                     "{ROW_ID} bulk read returned {} rows, expected {}",
                     addresses.len(),
                     self.num_docs
                 ),
             ));
         }
         if !use_bulk && addresses.len() != unique.len() {
             return Err(corrupt_docs(
                 &self.path,
                 format!(
                     "{ROW_ID} point read returned {} rows, expected {}",
                     addresses.len(),
                     unique.len()
                 ),
             ));
         }

         if use_bulk {
             Ok(doc_ids
                 .iter()
                 .map(|doc_id| addresses[doc_id.as_usize()])
                 .collect())
         } else {
             let by_doc_id = unique
-                .into_iter()
-                .zip(addresses)
+                .iter()
+                .copied()
+                .zip(addresses.iter().copied())
                 .collect::<std::collections::HashMap<_, _>>();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/documents.rs` around lines 631 - 657,
Update the address resolution logic around the address_column values in the bulk
and point branches to avoid eagerly calling to_vec() for all rows. Keep the bulk
path borrowing and indexing the slice returned by values(), while making only
the point-read path create any owned collection it requires and preserve its
existing length validation and mapping behavior.

rust/lance-index/src/scalar/inverted/documents.rs (2)

1578-1611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the error variant, not just the message.

This test verifies corruption is surfaced "not fallback", so it should confirm the returned error is actually the corruption variant (e.g. matching Error::CorruptFile/corrupt_file) in addition to the message substring. Message-only checks let a misclassified error (e.g. Error::IO) pass silently. Same applies to the other corruption tests here (document_column_nulls_are_reported_as_corruption, required_document_columns_validate_name_and_type, doc_lengths_validate_shape_total_and_memory).

As per coding guidelines: "Assert on both the error variant and the message content in tests; do not check only is_err()."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/documents.rs` around lines 1578 - 1611,
Update invalid_or_mismatched_stats_are_corruption_not_fallback and the related
corruption tests document_column_nulls_are_reported_as_corruption,
required_document_columns_validate_name_and_type, and
doc_lengths_validate_shape_total_and_memory to match the returned errors against
the corruption variant (such as Error::CorruptFile/corrupt_file), while
retaining the existing message-content assertions. Ensure each test verifies
both classification and descriptive error text rather than relying only on
is_err or substring checks.

Source: Coding guidelines

---

767-793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deduplicate the two identical DocId→address mapping blocks.

The resident-projection branch (Lines 767-779) and the remapper branch (Lines 780-793) differ only in how projection is obtained; the mapping closure and error message are copy-pasted. Collapsing them removes the risk of the two error paths drifting apart later.

♻️ Proposed refactor

-        if let Some(projection) = self.projection.get() {
-            return doc_ids
-                .iter()
-                .map(|&doc_id| {
-                    projection.address(doc_id).ok_or_else(|| {
-                        corrupt_docs(
-                            &self.path,
-                            format!("candidate DocId {} is not live", doc_id.get()),
-                        )
-                    })
-                })
-                .collect();
-        }
-        if self.remapper.is_some() {
-            let projection = self.projection().await?;
-            return doc_ids
-                .iter()
-                .map(|&doc_id| {
-                    projection.address(doc_id).ok_or_else(|| {
-                        corrupt_docs(
-                            &self.path,
-                            format!("candidate DocId {} is not live", doc_id.get()),
-                        )
-                    })
-                })
-                .collect();
-        }
+        let resident = self.projection.get().cloned();
+        let projection = match resident {
+            Some(projection) => Some(projection),
+            None if self.remapper.is_some() => Some(self.projection().await?),
+            None => None,
+        };
+        if let Some(projection) = projection {
+            return doc_ids
+                .iter()
+                .map(|&doc_id| {
+                    projection.address(doc_id).ok_or_else(|| {
+                        corrupt_docs(
+                            &self.path,
+                            format!("candidate DocId {} is not live", doc_id.get()),
+                        )
+                    })
+                })
+                .collect();
+        }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/documents.rs` around lines 767 - 793,
Deduplicate the DocId-to-address mapping in the surrounding method: first obtain
the projection from the resident cache or, when a remapper exists, await
projection(), then run one shared doc_ids.iter() mapping block using
projection.address and the existing corrupt_docs error. Preserve the current
branch behavior and return handling.

rust/lance-index/src/scalar/inverted/index.rs (1)

362-362: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the 64 MiB read-budget threshold.

MAX_CONCURRENT_ADDRESS_READ_BYTES gates deferred address-read fan-out but carries no comment on what the value represents or why 64 MiB was chosen.

📝 Suggested doc comment

+/// Upper bound on the total bytes of concurrent deferred address reads. Caps
+/// `address_read_concurrency` so a top-k spread across many partitions keeps
+/// its projection I/O bounded (~64 MiB) instead of scaling with partition count.
 const MAX_CONCURRENT_ADDRESS_READ_BYTES: usize = 64 * 1024 * 1024;

As per coding guidelines: "Add doc comments to magic constants, thresholds, and non-obvious transformation functions, explaining what the value represents and why it was chosen."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/index.rs` at line 362, Document
MAX_CONCURRENT_ADDRESS_READ_BYTES with a comment explaining that it limits
deferred address-read fan-out to a 64 MiB concurrent read budget and why this
threshold was selected. Keep the constant value and surrounding behavior
unchanged.

Source: Coding guidelines

rust/lance-index/src/scalar/inverted/wand.rs (1)

171-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

row_id field name no longer describes what it holds.

to_candidate(doc.row_id) feeds the heap key's row_id field into a generic candidate mapper — for the modern adapter this is a DocId, not a row id. Keeping the field named row_id after generalizing into_candidates over C is imprecise and could mislead future readers into assuming it's always a physical row identifier.

Please confirm the underlying ScoredDoc/heap-key struct definition (not shown in this diff) and consider renaming row_id to something like key/document_key to match the now-generic semantics.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/wand.rs` around lines 171 - 192, Confirm
the underlying ScoredDoc/heap-key definition and rename its generic identifier
field from row_id to an accurate name such as document_key, updating all
constructors, accesses, and heap-related uses including into_candidates.
Preserve the existing value passed to to_candidate while removing assumptions
that it is always a physical row identifier.